-
-
Notifications
You must be signed in to change notification settings - Fork 231
feat: add AccountTreeController
#5847
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…AccountGroupController
56bec57
to
e96d94e
Compare
@metamaskbot publish-preview |
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions.
|
packages/account-group-controller/src/AccountGroupController.test.ts
Outdated
Show resolved
Hide resolved
packages/account-group-controller/src/AccountGroupController.test.ts
Outdated
Show resolved
Hide resolved
packages/account-group-controller/src/AccountGroupController.test.ts
Outdated
Show resolved
Hide resolved
packages/account-group-controller/src/AccountGroupController.test.ts
Outdated
Show resolved
Hide resolved
packages/account-group-controller/src/AccountGroupController.test.ts
Outdated
Show resolved
Hide resolved
packages/account-group-controller/src/AccountGroupController.test.ts
Outdated
Show resolved
Hide resolved
packages/account-group-controller/src/AccountGroupController.ts
Outdated
Show resolved
Hide resolved
packages/account-group-controller/src/AccountGroupController.ts
Outdated
Show resolved
Hide resolved
packages/account-group-controller/src/AccountGroupController.ts
Outdated
Show resolved
Hide resolved
accountGroups: { | ||
groups: { | ||
// Wallet | ||
[accountGroup: AccountGroupId]: { | ||
// Multichain Account OR Account Group | ||
[accountSubGroup: AccountGroupId]: AccountId[]; // Blockchain Accounts | ||
}; | ||
}; | ||
}; | ||
accountGroupsMetadata: { | ||
[accountGroup: AccountGroupId]: AccountGroupMetadata; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a sync with @danroc @gantunesr:
We could use this state instead:
- Better locality with
metadata
- We would still persist everything (mainly because of the
metadata
) - We should probably rename this controller to
AccountWalletController
- We would have another
MultichainAccountController
(stateless) that would abstract high-level multichain accounts methods/capabilities on top of this controller for multichain accounts support
accountGroups: { | |
groups: { | |
// Wallet | |
[accountGroup: AccountGroupId]: { | |
// Multichain Account OR Account Group | |
[accountSubGroup: AccountGroupId]: AccountId[]; // Blockchain Accounts | |
}; | |
}; | |
}; | |
accountGroupsMetadata: { | |
[accountGroup: AccountGroupId]: AccountGroupMetadata; | |
}; | |
accountWallets: { | |
// Wallet | |
[accountWallet: AccountWalletId]: { | |
// Multichain Account OR Account Group | |
groups: [accountGroup: AccountGroupId]: { | |
accounts: AccountId[]; // Blockchain Accounts | |
metadata: Metadata; | |
}, | |
metadata: Metadata; | |
}; | |
}; |
@PatrykLucka WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you share a quick draft of what would be the use cases the MultichainAccountController
should be responsible for? I'm concerned about the over complexity that is being introduced around all the controllers that will manage accounts to their own extend and the still present accounts legacy code.
it does not need to be a new ADR, just a quick notion page will be enough to start setting the limits of each controller and how they interact between each other
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay.
My initial idea was to introduce a new MultichainAccountController
mainly to expose "multichain account capabilities" like:
- create/remove multichain account
- get a blockchain account from a multichain account (given a specific scope for example)
- get the associated wallet id or group id of a multichain account
The AccountTreeController
would be responsible for the hierarchy and structure of accounts.
The MultichainAccountController
would rely on the AccountTreeController
for the structure (and could also provide the "rule" being used to group multichain accounts together) AND provide multichain capabilities.
I haven't discussed any of those designs with anyone TBH, was just something I had in mind when thinking about the new initiative.
packages/account-group-controller/src/AccountGroupController.ts
Outdated
Show resolved
Hide resolved
packages/account-group-controller/src/AccountGroupController.ts
Outdated
Show resolved
Hide resolved
accountGroups: { | ||
groups: { | ||
// Wallet | ||
[accountGroup: AccountGroupId]: { | ||
// Multichain Account OR Account Group | ||
[accountSubGroup: AccountGroupId]: AccountId[]; // Blockchain Accounts | ||
}; | ||
}; | ||
}; | ||
accountGroupsMetadata: { | ||
[accountGroup: AccountGroupId]: AccountGroupMetadata; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you share a quick draft of what would be the use cases the MultichainAccountController
should be responsible for? I'm concerned about the over complexity that is being introduced around all the controllers that will manage accounts to their own extend and the still present accounts legacy code.
it does not need to be a new ADR, just a quick notion page will be enough to start setting the limits of each controller and how they interact between each other
packages/account-group-controller/src/AccountGroupController.ts
Outdated
Show resolved
Hide resolved
packages/account-group-controller/src/AccountGroupController.ts
Outdated
Show resolved
Hide resolved
packages/account-group-controller/src/AccountGroupController.ts
Outdated
Show resolved
Hide resolved
f740c42
to
63a0a51
Compare
packages/account-wallet-controller/src/AccountWalletController.ts
Outdated
Show resolved
Hide resolved
packages/account-wallet-controller/src/AccountWalletController.ts
Outdated
Show resolved
Hide resolved
packages/account-wallet-controller/src/AccountWalletController.ts
Outdated
Show resolved
Hide resolved
packages/account-wallet-controller/src/AccountWalletController.ts
Outdated
Show resolved
Hide resolved
packages/account-wallet-controller/src/AccountWalletController.ts
Outdated
Show resolved
Hide resolved
packages/account-wallet-controller/src/AccountWalletController.ts
Outdated
Show resolved
Hide resolved
…ount wallet naming
…eyring when entropy source is unknown
…controller) when naming keyring rule matches
@metamaskbot publish-preview |
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions.
|
@metamaskbot publish-preview |
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions.
|
@metamaskbot publish-preview |
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions.
|
AccountWalletController
AccountTreeController
@metamaskbot publish-preview |
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job! Just had some minor suggestions.
packages/account-tree-controller/src/AccountTreeController.test.ts
Outdated
Show resolved
Hide resolved
packages/account-tree-controller/src/AccountTreeController.test.ts
Outdated
Show resolved
Hide resolved
packages/account-tree-controller/src/AccountTreeController.test.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Elliot Winkler <[email protected]>
Co-authored-by: Elliot Winkler <[email protected]>
Co-authored-by: Elliot Winkler <[email protected]>
@metamaskbot publish-preview |
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions.
|
Explanation
New controller to group accounts based on some pre-defined rules.
References
Changelog
N/A
Checklist